Skip to content

eng-557 build centralized linting system for all repos #10

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

CDeltakai
Copy link
Contributor

@CDeltakai CDeltakai commented Mar 28, 2025

Description

This PR will implement a centralized linting system which will allow all MatrixAI repos to import from a single linting package (this one) and apply appropriate linting rules without further intervention. It should also allow downstream repos to extend the system in order to ignore certain rules or add project-specific rules.

Issues Fixed

Tasks

🔧 Repository Refactor

  • 1. Rename the repository from js-eslint to js-lint
  • 2. Update package.json:
    • 2.1 Change package name to @matrixai/lint
    • 2.2 Add a bin entry:
      "bin": {
        "lint": "dist/bin/lint.js"
      }

🧰 Lint Command Implementation

  • 3. Create src/bin/lint.ts
  • 4. Follow structure and logic of zeta.house/scripts/lint.mjs
  • 5. Implement lint runners for:
    • 5.1 eslint for JS/TS files
    • 5.2 prettier for markdown
    • 5.3 shellcheck for shell scripts
    • [ ] 5.4 clang-format for C/C++
    • [ ] 5.5 nixfmt for Nix files
    • [ ] 5.6 rustfmt for Rust files
  • 6. Ensure the script:
    • 6.1 Checks if each native dependency exists before executing
    • 6.2 Uses a fast, cross-platform commandExists utility

🧪 Add Utility Function

  • 7. Implement commandExists(programName: string): boolean that:
    • 7.1 Works on Linux, macOS, and Windows
    • 7.2 Is fast and lightweight
    • 7.3 Returns true/false for command existence
  • 7A. We need this package to be extendable by downstream repos as well so they can extend the package and ignore certain rules or implement custom rules if need be.

📦 Package Dependencies

  • 8. Add all relevant eslint, @typescript-eslint, prettier, and related dependencies
  • 9. Refer to zeta.house package.json as a baseline
  • 10. Do not include native tools like shellcheck or clang-format in package.json

🔁 Plugin De-emphasis

  • 11. Do not export an ESLint plugin as the main export
  • 12. Optionally retain plugin code for internal use only

🚀 Prepare for Release

  • 13. Tag the repo with version v0.0.1 (CI will handle publishing)

🌱 Downstream Integration

  • 14. Create a feature branch in js-logger
  • 15. Integrate @matrixai/lint and verify it runs correctly
  • 16. Optionally filter directories or extend the lint command if needed

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CDeltakai CDeltakai self-assigned this Mar 28, 2025
Copy link

linear bot commented Mar 28, 2025

ENG-557

@CDeltakai CDeltakai changed the title Feature eng 557 centralized linting system for all repos eng-557 build centralized linting system for all repos Mar 28, 2025
@CMCDragonkai
Copy link
Member

Let's get this merged after fixing those minor things, and then integrate into js-logger as a start.

@CMCDragonkai
Copy link
Member

Request @tegefaulkes to review once you are finished cleaning up this codebase in relation to conventions. Make sure to review js-logger as a template for how you should structure things, finish up the above reviews accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically functioning as a CLI program wrapping all our needs right?

We should consider using Commander to handle the arg parsing and logic. That said its a bit much to address right now so we can make a new issue for tracking it.

Comment on lines +45 to +49
console.error(
`--config points to “${explicitConfigPath}”, but that file does not exist.`,
);
process.exit(1); // Hard‑fail; nothing to lint against
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we need to use a similar error structure to Polykey. This works but I'd rather have an explicit error that gets thrown and the message/exit code is handled automatically. That would be part of a Commander conversion if we do it.

Comment on lines 103 to 105

const cfg = rawCfg as { tsconfigPaths?: unknown; forceInclude?: unknown };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do this, You're throwing type checking out of the window. It's also kinda pointless? You're saying an arbitray JSON object May have these keys and they are unknown? That's already true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is is a json file with an expected format then we need proper validation. Polykey already uses https://www.npmjs.com/package/ajv to apply schema to it's status file. We can use the same thing here.

This may be another add on issue.

Copy link
Contributor Author

@CDeltakai CDeltakai Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't do this, You're throwing type checking out of the window. It's also kinda pointless? You're saying an arbitray JSON object May have these keys and they are unknown? That's already true.

I've changed this to use a partial type with RawMatrixCfg:

type RawMatrixCfg = Partial<{
  tsconfigPaths: unknown;
  forceInclude: unknown;
}>; 

and then casting it to this type with the asRawMatrixCfg method:

function asRawMatrixCfg(v: unknown): RawMatrixCfg | undefined {
  return typeof v === 'object' && v !== null ? (v as RawMatrixCfg) : undefined;
}

as a basic type guard. I think this will fix this issue in the short term but I'll keep this open in case we want a proper issue with the proper validation fix.

…self and removed the default eslint.config.mjs since its not needed anymore
…aving an invalid config json hard fail instead of falling back and removed unnecessary arrow functions
fixed potential race condition as well as eslint exiting the process prematurely if any error was found, preventing shellcheck or prettier from running
… and removed unnecessary interface. removed ignoring files console log
… removed the main index.ts since nothing from there is being exported anymore. made a seperate plugin folder for the matrixai eslint plugin that contains our custom rules
@CDeltakai CDeltakai merged commit cb8322c into staging Apr 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants